-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Rust] Some rust cleanups #6116
Conversation
* Turn off default features for bindgen * Upgrade some deps for smaller total dep tree * Switch (/complete switch) to thiserror * Remove unnecessary transmutes
@@ -32,4 +32,4 @@ ndarray = "0.12" | |||
enumn = "^0.1" | |||
|
|||
[build-dependencies] | |||
bindgen = "0.51" | |||
bindgen = { version="0.51", default-features=false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @kazum wanted this on for building the WASM backend, can you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default-features of bindgen
pulls in clap
which has a pretty big (albeit build-dep
) dependency load. So it'd be good to trim if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was necessary before, but now I confirm wasm32 can be compiled successfully without it. I'm not sure why.
@binarybana Could you revive a wasm32 test to make sure that this PR doesn't break wasm32 build?
diff --git a/tests/scripts/task_rust.sh b/tests/scripts/task_rust.sh
index 6d159f6..7cdfb1a 100755
--- a/tests/scripts/task_rust.sh
+++ b/tests/scripts/task_rust.sh
@@ -71,11 +71,11 @@ cd tests/test_tvm_dso
cargo run
cd -
-# # run wasm32 test
-# cd tests/test_wasm32
-# cargo build
+# run wasm32 test
+cd tests/test_wasm32
+cargo build
# wasmtime $RUST_DIR/target/wasm32-wasi/debug/test-wasm32.wasm
-# cd -
+cd -
# run nn graph test
cd tests/test_nn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up @kazum all the big changes to Rust should be done. I will be sending a bunch of doc updates in coming weeks after I finish landing new Relay error reporting. Max and I have been using the bindings at OctoML for some experiments.
LGTM, @mwillsey can you review as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I noticed a small build error I introduced in the last rebase. Will fix and push when I get a second, but wanted to flag it first. |
Okay, builds and tests pass now. |
Thanks Jason! |
* Some rust cleanups * Turn off default features for bindgen * Upgrade some deps for smaller total dep tree * Switch (/complete switch) to thiserror * Remove unnecessary transmutes * Fix null pointer assert * Update wasm32 test
* Some rust cleanups * Turn off default features for bindgen * Upgrade some deps for smaller total dep tree * Switch (/complete switch) to thiserror * Remove unnecessary transmutes * Fix null pointer assert * Update wasm32 test
* Some rust cleanups * Turn off default features for bindgen * Upgrade some deps for smaller total dep tree * Switch (/complete switch) to thiserror * Remove unnecessary transmutes * Fix null pointer assert * Update wasm32 test
* Some rust cleanups * Turn off default features for bindgen * Upgrade some deps for smaller total dep tree * Switch (/complete switch) to thiserror * Remove unnecessary transmutes * Fix null pointer assert * Update wasm32 test
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.